fix(chat): send the double-submit CSRF token on attachment uploads - #3640

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

src/agent/react/use-chat/use-chat.csrf.test.tsx (1)

110-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a negative control so the suite cannot pass vacuously.

Every edge assertion expects 200. If CsrfHandler stopped enforcing, or csrfCtx() stopped enabling csrf, all three protected-request tests would still pass. Add one case that sends a mutation without the token through the same edge and expect 403. This pins the enforcement that the positive cases rely on.

🧪 Proposed control test

+  it("rejects the same mutation when the client omits the token", async () => {
+    const restoreDom = installDom();
+    loadDocumentCookie();
+    const edge = installCsrfEdge(() => agUiResponse());
+    try {
+      await fetch("/api/ag-ui", {
+        method: "POST",
+        headers: { "Content-Type": "application/json" },
+        body: "{}",
+      });
+
+      assertEquals(edge.statuses.get("POST /api/ag-ui"), 403);
+    } finally {
+      edge.restore();
+      restoreDom();
+    }
+  });

Also applies to: 144-167

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/agent/react/use-chat/use-chat.csrf.test.tsx` around lines 110 - 134, Add
a negative-control test using installCsrfEdge that sends a protected mutation
without the CSRF token and asserts a 403 response. Keep it alongside the
existing protected-request cases so the suite verifies CsrfHandler enforcement
and csrfCtx() configuration rather than passing vacuously.

src/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsx (3)

143-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move test setup inside try so patched globals always restore.

Each test patches globalThis before the try block. loadDocumentCookie throws by design when the CSRF cookie is HttpOnly. installCsrfEdge and renderAttachments can also throw. In those cases restoreDom never runs, so jsdom window, document, and the patched fetch leak into the following tests in the same process. The failure then appears in an unrelated test.

Acquire the restore function first, then perform the remaining setup inside try.

♻️ Proposed structure for each test

     const restoreDom = installDom();
-    loadDocumentCookie();
-    const edge = installCsrfEdge((req) =>
-      req.method === "GET"
-        ? Response.json({ items: [] })
-        : Response.json({ id: "up-1", name: "a.txt", url: "/files/a.txt", size: 1 })
-    );
-    const view = renderAttachments("/api/uploads");
+    let edge: ReturnType<typeof installCsrfEdge> | undefined;
+    let view: ReturnType<typeof renderAttachments> | undefined;
     try {
+      loadDocumentCookie();
+      edge = installCsrfEdge((req) =>
+        req.method === "GET"
+          ? Response.json({ items: [] })
+          : Response.json({ id: "up-1", name: "a.txt", url: "/files/a.txt", size: 1 })
+      );
+      view = renderAttachments("/api/uploads");
       view.attachments().upload([new File(["a"], "a.txt", { type: "text/plain" })]);
       await waitFor(() => edge.statuses.has("POST /api/uploads"));
 
       assertEquals(edge.statuses.get("POST /api/uploads"), 200);
     } finally {
-      view.unmount();
-      edge.restore();
+      view?.unmount();
+      edge?.restore();
       restoreDom();
     }

The same pattern applies to the removal test at lines 164-166 and the cross-origin test at lines 187-188.

Also applies to: 164-166, 187-188

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsx`
around lines 143 - 150, Move loadDocumentCookie, installCsrfEdge, and
renderAttachments setup inside the try block for each affected test, including
the removal and cross-origin tests, while acquiring restoreDom first. Keep
cleanup in finally so restoreDom always runs when any setup step throws.

---

106-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The simulated edge does not reproduce the multipart body.

installCsrfEdge builds a native Request from init.body, which is a jsdom FormData instance. A native Request does not recognize a FormData from another realm, so the body is coerced instead of encoded as multipart. The CSRF assertions do not read the body, so the current tests still pass. If a later test asserts on upload content, the body will be wrong.

String(input) also fails if a caller passes a Request or URL. Normalize the input instead.

♻️ Proposed hardening of the fetch stub

   globalThis.fetch = async (input, init) => {
-    const url = new URL(String(input), document.baseURI);
+    const raw = input instanceof Request ? input.url : String(input);
+    const url = new URL(raw, document.baseURI);
     const headers = new Headers(init?.headers);
     if (document.cookie) headers.set("cookie", document.cookie);
     const req = new Request(url, {
       method: init?.method ?? "GET",
       headers,
       body: init?.body as BodyInit | undefined,
     });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsx`
around lines 106 - 114, Update installCsrfEdge’s globalThis.fetch stub to
normalize input via the existing Request/URL-aware URL handling instead of
String(input). Preserve jsdom FormData when constructing the native Request by
converting or re-encoding the cross-realm form data as multipart, including the
correct boundary-bearing Content-Type, so upload bodies remain valid for future
assertions.

---

186-206: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Consider covering the cross-origin removal request too.

This test proves that the upload POST omits the token for a cross-origin endpoint. The DELETE removal path calls csrfMutationHeaders(target, ...) with a different URL built by setQueryParameter, so it has its own origin computation. A cross-origin DELETE case would guard that second call site.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/react/components/chat/chat/hooks/use-uploads-registry.csrf.test.tsx`
around lines 186 - 206, Add a separate test alongside the existing cross-origin
upload test that exercises attachment removal and verifies the DELETE request
sends no page CSRF token. Use a cross-origin upload endpoint, capture headers
for DELETE, remove the uploaded attachment, and assert the captured x-csrf-token
is null, covering the URL produced through setQueryParameter and its
csrfMutationHeaders call.